Implement heterogeneous Association Map based on SoA blocks - #49995
Implement heterogeneous Association Map based on SoA blocks#49995sbaldu wants to merge 3 commits into
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49995/47797 |
|
A new Pull Request was created by @sbaldu for master. It involves the following packages:
The following packages do not have a category, yet: DataFormats/AssociationMap @cmsbuild, @fwyzard, @makortel can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
type ngt |
| #include <cstdint> | ||
| #include <type_traits> | ||
|
|
||
| namespace ticl { |
There was a problem hiding this comment.
Is this utility specific to TICL or intended for more general usage?
There was a problem hiding this comment.
As of now the main usage is TICL
There was a problem hiding this comment.
Would it be feasible to include this code in a package that already uses TICL?
Or, the motivation to place this code in its own package is not clear.
There was a problem hiding this comment.
Thanks for the suggestion. I've now put the map under a new DataFormats/TICL package
There was a problem hiding this comment.
With @felicepantaleo we decided to add a new package for TICL data formats and we'll start moving the structures specific for TICL there
There was a problem hiding this comment.
With @felicepantaleo we decided to add a new package for TICL data formats and we'll start moving the structures specific for TICL there
Ok. I guess at least @cms-sw/reconstruction-l2 should sign it? What about @cms-sw/heterogeneous-l2?
There was a problem hiding this comment.
What about @cms-sw/heterogeneous-l2?
This too, I guess 🤷🏻♂️
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49995/47813 |
a9ddafa to
9b406b0
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49995/47834 |
|
I think that fixing the previous off-by-one issues has introduced some new unwanted behaviour (that escaped the current testing). Now the
The override of the SoABlocks sizes in AssociationMap makes the two disagree so that building a map with
I think this "disagreement" might have multiple repercussions (e.g. probably in the |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49995/50648
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49995/50664
|
|
please test |
|
-1 Failed Tests: RelVals-AMD_MI300X DAS Queries: The DAS query tests failed, see the summary page for details. Failed RelVals-AMD_MI300X
Expand to see more relval errors ...Comparison SummarySummary:
NVIDIA_H100 Comparison SummarySummary:
NVIDIA_L40S Comparison SummarySummary:
NVIDIA_T4 Comparison SummarySummary:
Max Memory Comparisons exceeding threshold NVIDIA_H100@cms-sw/core-l2 , I found 1 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
Max Memory Comparisons exceeding threshold NVIDIA_T4@cms-sw/core-l2 , I found 1 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
|
|
ignore tests-rejected with ib-failure |
I added a test specifically for the deepCopy and it works as expected. With @Electricks94 we also tested reading the map from ROOT files, and that also works correctly, producing a map with the correct content and sizes. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49995/50673
|
There was a problem hiding this comment.
Can you add or modify the tests to check some more cases:
-
the map is not completely full (can it happen for the keys, or only for the values ?)
-
the map is exactly full (I think all current tests assume this)
-
the keys overflow (should assert ?)
-
the values overflow (should assert ?)
-
the number of keys is 30
-
the number of keys is 31 (so n+1 is 32 which is on the alignment boundary)
-
the number of keys is 32
-
different keys have different number of elements
There was a problem hiding this comment.
In the effort to increase the robustness of the tests I would also opt to add a ROOT streamer test that stores and reads a map. There we rely on the interplay between SoABlocks and the layouts it contains. Everything is correct as it is but we should make sure that future developements don't destroy anything here. Especially, since we plan to support RNTuple soon things might change and I think it will be important to test that these changes don't affect anything that is developed in this PR
There was a problem hiding this comment.
I have written a test case for this a while ago here. I think it just needs to be added to right place in this PR if found useful
There was a problem hiding this comment.
I added the required tests
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49995/50674
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49995/50675
|
|
I am lost among the discussion in #49995 (comment) and #49995 (comment) . I agree with @Parsifal-2045 that having an potentially public inconsistent interface is Bad. I understand that we need I would like to understand what code would produce inconsistent results, and discuss how to address the issue. |
@Parsifal-2045 @fwyzard /* Helper to implement View as derived from ConstView in SoABlocks implementation */
template <bool RESTRICT_QUALIFY, cms::soa::RangeChecking::Mode RANGE_CHECKING>
SOA_HOST_DEVICE SOA_INLINE static ViewTemplate<RESTRICT_QUALIFY, RANGE_CHECKING> const_cast_View(
ConstViewTemplate<RESTRICT_QUALIFY, RANGE_CHECKING> const& view) {
return ViewTemplate<RESTRICT_QUALIFY, RANGE_CHECKING>{
_ITERATE_ON_ALL_COMMA(_DECLARE_CONST_CAST_VIEWS, ~, __VA_ARGS__)};
} which then calls #define _DECLARE_CONST_CAST_VIEWS_IMPL(VALUE_TYPE, NAME, LAYOUT_NAME)
(LayoutFor<LAYOUT_NAME>::const_cast_View(view.NAME())) |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49995/50688
|
9 participants
PR description:
This PR implements an association map based on SoA blocks. In order to provide the accessors to the map's properties and content, the PR also enables the definition of SoA view methods.
PR validation:
The PR includes a test of the construction and fill of the map and checks the correctness of the content and the accessor methods.
FYI @felicepantaleo